[PAL/Linux-SGX] Fix Trusted Files degenerating to Allowed Files on fork #1796
Conversation
dimakuv
force-pushed
the
dimakuv/fix-trusted-files-as-allowed
branch
from
4292ee6 to
0277a1b
Compare
There was a problem hiding this comment.
Reviewed 13 of 13 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
Previously, Trusted Files feature had the following bug: after fork, the metadata of currently-opened-in-parent-process TFs (SHA256 hashes for each chunk of the file) was not available in the child SGX enclave. This effectively degenerated all currently-opened TFs into Allowed Files, and thus the child enclave lost integrity guarantees in these TFs. An attacker could substitute the TF contents on the storage with arbitrary code/data, and the child enclave would not notice this. This bug was exposed only in the child process (child SGX enclave) and only in these scenarios: - fork without execve: the child continues reading from one of the file descriptors pointing to a trusted file opened before `fork()`; - fork with execve: the child reads from an inherited file descriptor pointing to a trusted file. This commit also adds a test to verify this bug is fixed. Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
dimakuv
force-pushed
the
dimakuv/fix-trusted-files-as-allowed
branch
from
0277a1b to
d82e885
Compare
There was a problem hiding this comment.
Reviewable status: 11 of 13 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin)
pal/src/host/linux-sgx/pal_host.h line 56 at r3 (raw file):
/* below fields are used only for trusted files */ sgx_chunk_hash_t* chunk_hashes; /* array of hashes of file chunks */ void* umem; /* valid only when chunk_hashes != NULL and size > 0 */
In a previous iteration, I forgot that umem can be legitimately NULL if the underlying Trusted File is empty (then it cannot be mmapped, since there are no contents to mmap).
My misunderstanding led to me checking assert(handle->file.umem) everywhere. And on our Python tests it failed, because Python has some trusted files like __init__.py which are empty. So I fixed my asserts to only check umem when necessary.
I expanded the comment to not forget about this in the future.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
There was a problem hiding this comment.
Reviewed 10 of 13 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
Previously, Trusted Files feature had the following bug: after fork, the metadata of currently-opened-in-parent-process TFs (SHA256 hashes for each chunk of the file) was not available in the child SGX enclave. This effectively degenerated all currently-opened TFs into Allowed Files, and thus the child enclave lost integrity guarantees in these TFs. An attacker could substitute the TF contents on the storage with arbitrary code/data, and the child enclave would not notice this.
This bug was exposed only in the child process (child SGX enclave) and only in these scenarios:
fork();This PR also adds a test to verify this bug is fixed.
How to test this PR?
LibOS test (based on a GDB script) is added.
This change is